-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send notification mail for circle invitation #1349
base: master
Are you sure you want to change the base?
Conversation
7d4e51c
to
9116356
Compare
Signed-off-by: Jonas Heinrich <heinrich@synyx.de>
9116356
to
ce6de43
Compare
Will be happy for a review, it's my first contribution to Circles @ArtificialOwl 😊 |
Happy for review @susnux :) |
} catch (Exception $e) { | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens during this exception? You probably want to write this on the logs?
$message->setHtmlBody($emailTemplate->renderHtml()); | ||
$message->setTo([$recipient]); | ||
|
||
$this->mailer->send($message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know but it could be possible to build all the mail templates and call the send function with an array of messages? (Related to the loop that calls sendMailInvitation
on linr 122)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few inline comments.
$author = $member->getInvitedBy()->getDisplayName(); | ||
} else { | ||
$author = 'someone'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
$author = ucfirst($author); |
I see in the screen shot that test
started with lower-case so, we want to start the statement and name in this case with caps.
Not sure if this is the best way to implement this, currently we using notifications, but activities would support mail out of the box (users can dis / enable mails). So we would not need to implement any mail logic here. @ArtificialOwl you implemented the activities already but commented out it in the app info xml, should we try to enable them again? |
I am currently working on it |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
Get notified if someone invites you into a circle.
Fixes #235